fix: Dynamically compute default zoom/center for map traces#7884
fix: Dynamically compute default zoom/center for map traces#7884camdecoster wants to merge 12 commits into
Conversation
| }); | ||
|
|
||
| // Auto-frame the initial view to the data | ||
| if (containerIn.center === undefined && containerIn.zoom === undefined) { |
There was a problem hiding this comment.
Should skip this step if bounds is defined.
Also, I'm not positive, but I think we should test containerOut rather than containerIn, so that if the user supplies an invalid center or zoom value, we ignore it and compute the auto-bounds anyway.
Edit: Never mind, I misunderstood the bounds param. I do think we should test on containerOut though.
| if (containerIn.center === undefined && containerIn.zoom === undefined) { | |
| if (containerOut.center === undefined && containerOut.zoom === undefined) { |
There was a problem hiding this comment.
If we test containerOut, center/zoom will always be set because coerce supplies default values for those attributes. If containerIn doesn't have center/zoom, that's a signal that we need to provide bounds for the map. If the user supplies junk, this will still work because the call to getMapFitBounds won't run and coerce already took care of the junk values.
There was a problem hiding this comment.
Oh yeah of course we can't use containerOut, that makes sense.
But that does mean auto-bounds won't be applied if the user supplies junk values for center and zoom, right?
(That seems like the wrong behavior to me, but I could be convinced otherwise.)
There was a problem hiding this comment.
I'm deferring to the user in this case. If they explicitly set values that would otherwise be set by auto-fitting, I opted to skip auto-fitting. If you want to provide garbage values, that's up to you. We could update that behavior in the future, but I think we should trust the user for now.
| opts._input.center = opts.center = center; | ||
| opts._input.zoom = opts.zoom = zoom; |
There was a problem hiding this comment.
This will change the center and zoom values stored in fullLayout.map, right? I'm not totally sure it's OK to modify fullLayout at this point.
There was a problem hiding this comment.
modifying fullLayout at this point , This is actually the same pattern used a few lines below in the moveend
handler, where user pan/zoom writes back to opts._input.center , opts._input.zoom , etc. We need to do it here because MapLibre resolves the bounding box into a concrete center/zoom internally , if we don't capture those values, subsequent updateLayout calls would reset the view to the schema defaults (center 0,0 / zoom 1) instead of the auto-framed position.
please correct me if i am wrong.
There was a problem hiding this comment.
It's okay because that's the pattern we're already using in moveend down below. Since we're already mutating these values during pan/zoom, this seemed like an acceptable way to save state.
| self.viewInitial = { | ||
| center: Lib.extendFlat({}, center), | ||
| bearing: opts.bearing, | ||
| pitch: opts.pitch, | ||
| zoom | ||
| }; |
There was a problem hiding this comment.
I don't love this, since in theory viewInitial is already being set in the plot() function and this feels like an end run around that logic and ripe for bugs. But I'm open to arguments about why this is the best/only way to handle viewInitial in the auto-bounds case.
There was a problem hiding this comment.
The issue is timing: viewInitial normally gets set in
plot() before the map's load event fires, at which point opts.center and opts.zoom still hold the schema
defaults since MapLibre hasn't resolved the bounds yet. Without this override, double-clicking "reset view" would
snap to center=(0,0), zoom=1 instead of the auto-framed view the user actually saw. Moving the viewInitial
assignment into the load callback for all cases would be cleaner but felt like too big a refactor for this PR -
happy to hear if you see a better approach though!
There was a problem hiding this comment.
This update is necessary to make the Reset modebar button and map double click callbacks use the correct values. This seemed like a reasonable place to update the data, but let me explore reworking the plot function.
Description
Use lat/lon points to dynamically compute the default zoom/center for
scattermapanddensitymaptraces.Closes #7674.
Changes
handleDefaultsto dynamically computer map boundsScreenshots
Testing
{ "data": [ { "hovertext": ["San Marino", "Cairo", "Istanbul", "Trondheim"], "lat": [43.9360958, 30.06263, 41.01384, 63.43049], "lon": [12.4417702, 31.24967, 28.94966, 10.39506], "marker": { "color": "#f00" }, "mode": "markers", "type": "scattermap" } ], "layout": { "width": 900, "height": 600 } }Notes